Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support specify alias for package in core #419

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

lijunchen
Copy link
Contributor

No description provided.

Copy link

Based on the provided git diff output, here are three issues or potential improvements that can be observed:

  1. Multiple Calls to read_package_desc_file_in_dir:

    • In the run_run_internal function of crates/moon/src/cli/run.rs, there is a redundant call to read_package_desc_file_in_dir. The function first reads the module description (mod_desc) and then reads the package description using the module's name. This could be optimized to avoid unnecessary file reads.
  2. Potential Logic Issue in Import Handling:

    • In crates/moonutil/src/package.rs, the logic for handling imports and distinguishing between core and non-core imports seems to be duplicated across different types of imports (regular, wbtest, and test imports). This could lead to maintenance issues if the logic needs to change, as it would need to be updated in multiple places.
  3. Error Handling in read_module_from_json:

    • The read_module_from_json function in crates/moonutil/src/common.rs uses ? to propagate errors. However, if there's a need to provide more specific context or additional handling for certain errors (e.g., file not found, invalid JSON), it might be beneficial to add more granular error handling.

These observations are based on the provided diff and the context of the code changes. They suggest potential areas for optimization and improvement in the codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant